[PDO] fix: Explicitly convert PDO queries to utf-8 #296
Conversation
|
Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one. |
John98Zakaria
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
=========================================
Coverage 82.68% 82.68%
Complexity 949 949
=========================================
Files 89 89
Lines 3807 3807
=========================================
Hits 3148 3148
Misses 659 659
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
John98Zakaria
changed the title
|
I fixed the php-stan warning for my code. 🙏 |
composer.json
Outdated
| @@ -7,7 +7,8 @@ | |||
| "prefer-stable": true, | |||
| "require": { | |||
| "php": "^7.4 || ^8.0", | |||
| "ext-json": "*" | |||
| "ext-json": "*", | |||
| "ext-mbstring": "*" | |||
There was a problem hiding this comment.
can we rely on there being a polyfill available here (https://github.com/open-telemetry/opentelemetry-php/blob/main/src/API/composer.json#L23) and move mbstring to a suggestion (if it's better/more performant/etc) ?
There was a problem hiding this comment.
Such changes should also be added to src/Instrumentation/PDO/composer.json 👍
There was a problem hiding this comment.
-
I removed the requirement from the root composer json
-
Added
symfony/polyfill-mbstringas a requirement -
Added
ext-mbstringthe extension as a suggested
|
We have some integration tests for this package, could you please take a look and see if they can be enhanced to exercise this functionality? |
|
I added tests that check for utf-8 encoding |
This explicitly converts queries to utf-8 to allow them to be sent over grpc
I am not sure what the consequences are for Chinese or Arabic queries 😅 or any other non Latin alphabet language
Closes: open-telemetry/opentelemetry-php#1381